Modifications to enable Offline Signing in monero-gui and possible... #9492

DiosDelRayo · 2024-09-27T07:45:11Z

… other projects via UR or other mediums of exchange.

This PR is part of the CCS Monero Signer Resurrection (milestone 4). The modifications are needed to get the blobs of the offline signing data exchange without writing and reading from disk. To merge UR additions to monero-gui, this needs to be merged first.

   Added the following signatures in the mention files:

   wallet/wallet2.h
   +    std::string export_key_images_string(bool all = false) const;
   +    uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent);
   M    bool wallet2::export_key_images(const std::string &filename, bool all) const

   wallet/api/pending_transaction.h
   +    std::string commit_string() override;

   wallet/api/unsigned_transaction.h
   +    std::string signAsString() override;

   wallet/api/wallet.h:
   +    bool submitTransactionFromString(const std::string &fileName) override;
   +    virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) override;
   +    std::string exportKeyImagesAsString(bool all = false) override;
   +    bool importKeyImagesFromString(const std::string &data) override;
   +    std::string exportOutputsAsString(bool all = false) override;
   +    bool importOutputsFromString(const std::string &data) override;

   wallet/api/wallet2_api.h
   +    virtual std::string commit_string() = 0;
   +    virtual std::string signAsString() = 0;
   +    virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) = 0;
   +    virtual bool submitTransactionFromString(const std::string &fileName) = 0;
   +    virtual bool importKeyImagesFromString(const std::string &data) = 0;
   +    virtual std::string exportOutputsAsString(bool all = false) = 0;
   +    virtual bool importOutputsFromString(const std::string &data) = 0;
   +    uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent);

   And the implementations in:

   wallet/wallet2.cpp
   wallet/api/pending_transaction.cpp
   wallet/api/unsigned_transaction.cpp
   wallet/api/wallet.cpp

   The method `bool wallet2::export_key_images(const std::string &filename, bool all) const` is modified to
   use `std::string export_key_images_string(bool all = false) const;` to get the string to write to the file.
   IMO that would be the perfect way to do it everywhere, but in the other methods it would require more modifications, so the other I duplicated and removed the part writing to the file and return instead a std::string, or
   use a std::string for the actual payload instead of a file path.

   One thing to mention is I remove in one or two log messages the filename, and the other is in `export_key_images` probably(almost sure) is now the performance messed up.

   This modifications was done to get all the necessary data for offline signing via UR or any other channel not
   using files as medium. IMO it had been better to not implement the filehandling direct in wallet2 or in the wallet api but rather in monero-wallet-cli and monero-gui itself, but it is like it is.

…ther projects via UR or other mediums of exchange. Added the following signatures in the mention files: wallet/wallet2.h + std::string export_key_images_string(bool all = false) const; + uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent); M bool wallet2::export_key_images(const std::string &filename, bool all) const wallet/api/pending_transaction.h + std::string commit_string() override; wallet/api/unsigned_transaction.h + std::string signAsString() override; wallet/api/wallet.h: + bool submitTransactionFromString(const std::string &fileName) override; + virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) override; + std::string exportKeyImagesAsString(bool all = false) override; + bool importKeyImagesFromString(const std::string &data) override; + std::string exportOutputsAsString(bool all = false) override; + bool importOutputsFromString(const std::string &data) override; wallet/api/wallet2_api.h + virtual std::string commit_string() = 0; + virtual std::string signAsString() = 0; + virtual UnsignedTransaction * loadUnsignedTxFromString(const std::string &unsigned_filename) = 0; + virtual bool submitTransactionFromString(const std::string &fileName) = 0; + virtual bool importKeyImagesFromString(const std::string &data) = 0; + virtual std::string exportOutputsAsString(bool all = false) = 0; + virtual bool importOutputsFromString(const std::string &data) = 0; + uint64_t import_key_images_string(const std::string &data, uint64_t &spent, uint64_t &unspent); And the implementations in: wallet/wallet2.cpp wallet/api/pending_transaction.cpp wallet/api/unsigned_transaction.cpp wallet/api/wallet.cpp The method `bool wallet2::export_key_images(const std::string &filename, bool all) const` is modified to use `std::string export_key_images_string(bool all = false) const;` to get the string to write to the file. IMO that would be the perfect way to do it everywhere, but in the other methods it would require more modifications, so the other I duplicated and removed the part writing to the file and return instead a std::string, or use a std::string for the actual payload instead of a file path. One thing to mention is I remove in one or two log messages the filename, and the other is in `export_key_images` probably(almost sure) is now the performance messed up. This modifications was done to get all the necessary data for offline signing via UR or any other channel not using files as medium. IMO it had been better to not implement the filehandling direct in wallet2 or in the wallet api but rather in monero-wallet-cli and monero-gui itself, but it is like it is.

@tobtoht

and needs to get enabled with `WITH_OTS_UR` to compile with UR. This update depends on [monero PR](monero-project/monero#9492). It cannot work without modifications on monero core! @tobtoht also mentions that build with static Qt5 will not work. I compiled and tested exclusively with ubuntu 24.04 in Qt Creator with Qt 5.15.2

sneurlax

Concept ACK and the code changes LGTM for the most part. Functions are separated, errors are handled well, and documentation via inline comments is in-line with the status quo. You could add more comments in areas, but the context around your changes also lacks comments, so they're in the established style in those cases--but could nevertheless be commented and I think in that case breaking convention/style would be justified. Unit tests could also be added, although this could be a good first issue for a subsequent contributor after this is merged.

SNeedlewoods

Ran out of time for a full review today, but for the first pass it's looking mostly good, left a few comments in case you want to look into things already before I finish the review.

Thanks for your contribution.

SNeedlewoods · 2024-10-09T20:38:51Z

src/wallet/api/pending_transaction.cpp

@@ -162,6 +162,44 @@ bool PendingTransactionImpl::commit(const std::string &filename, bool overwrite)
    return m_status == Status_Ok;
 }

+std::string PendingTransactionImpl::commit_string()


Other methods in this file (and in the wallet API in general) are written in camelCase instead of snake_case.
And I would suggest a name closer to the wallet2 origin like dumpTxToStr().

SNeedlewoods · 2024-10-09T20:39:18Z

src/wallet/api/pending_transaction.cpp

+    } catch (const tools::error::daemon_busy&) {
+        // TODO: make it translatable with "tr"?
+        m_errorString = tr("daemon is busy. Please try again later.");
+        m_status = Status_Error;
+    } catch (const tools::error::no_connection_to_daemon&) {
+        m_errorString = tr("no connection to daemon. Please make sure daemon is running.");
+        m_status = Status_Error;
+    } catch (const tools::error::tx_rejected& e) {
+        std::ostringstream writer(m_errorString);
+        writer << (boost::format(tr("transaction %s was rejected by daemon with status: ")) % get_transaction_hash(e.tx())) <<  e.status();
+        std::string reason = e.reason();
+        m_status = Status_Error;
+        m_errorString = writer.str();
+        if (!reason.empty())
+          m_errorString  += string(tr(". Reason: ")) + reason;
+    } catch (const std::exception &e) {
+        m_errorString = string(tr("Unknown exception: ")) + e.what();
+        m_status = Status_Error;
+    } catch (...) {
+        m_errorString = tr("Unhandled exception");
+        LOG_ERROR(m_errorString);
+        m_status = Status_Error;
+    }


Seems these catchs were copied from the commit() method, but are mostly not needed here.
Only tools::error::wallet_internal_error can get thrown in get_construction_data_with_decrypted_short_payment_id() (which gets called in m_wallet.m_wallet->dump_tx_to_str(m_pending_tx);), afaict without looking too deep.

SNeedlewoods · 2024-10-09T20:39:24Z

src/wallet/api/pending_transaction.cpp

+        LOG_ERROR(m_errorString);
+        m_status = Status_Error;
+    }
+    m_wallet.startRefresh();


Don't think this is needed.

SNeedlewoods · 2024-10-09T20:40:21Z

src/wallet/api/wallet.cpp

@@ -1134,6 +1134,27 @@ UnsignedTransaction *WalletImpl::loadUnsignedTx(const std::string &unsigned_file
  return transaction;
 }

+UnsignedTransaction *WalletImpl::loadUnsignedTxFromString(const std::string &data) {
+  clearStatus();
+  UnsignedTransactionImpl * transaction = new UnsignedTransactionImpl(*this);


Reminds me of this issue: #9461

SNeedlewoods · 2024-10-09T20:41:38Z

src/wallet/wallet2.cpp

@@ -13262,9 +13262,8 @@ crypto::public_key wallet2::get_tx_pub_key_from_received_outs(const tools::walle
  return tx_pub_key;
 }

-bool wallet2::export_key_images(const std::string &filename, bool all) const
+std::string wallet2::export_key_images_string(bool all) const


From how I understand things at this time, I fully support to add this function.

For those who like to read, here is a snippet from the notes I took while working on the wallet API:

I am wondering, for the wallet2::export_key_images() (src) that's used by the wallet-rpc (here), afaiui it sends the key images and signatures in clear text, wouldn't it be safer to change it, so it returns a string of the encrypted key images and signatures?
If my concern is valid, I would not add the export_key_images() function mentioned above to the API, but propose to add another std::string export_key_images(bool all) in wallet2 and add that to the API instead, it would return an encrypted string of key images and signatures as created here (magic + ciphertext).
That should be safer to use for the wallet-rpc compared to what it's using now and I assume it would be the most versatile for every client using the wallet API.

SNeedlewoods

Round 2, review complete.
From this round the only relevant change for code flow is setStatus() to setStatusError().
Other things may be more subjective and/or only style related.

src/wallet/api/wallet.cpp

src/wallet/api/wallet.h

SNeedlewoods · 2024-10-14T13:18:56Z

src/wallet/api/wallet2_api.h

@@ -161,6 +163,11 @@ struct UnsignedTransaction
    * return - true on success
    */
    virtual bool sign(const std::string &signedFileName) = 0;
+   /*!
+    * @brief sign - Sign txs and return as string


Nitpick: @brief sign should match the function name like: @brief signAsString()

src/wallet/api/wallet2_api.h

(Method name: reason) signTxToStr: gets added in monero-project#9492 as `signAsString()` loadTx: not needed, all it does is `loadFromFile()` and `parseTxFromStr()` estimateBacklog(min_tx_weight, max_tx_weight, fees): not needed, calls `estimateBacklog(fee_levels)`

DiosDelRayo mentioned this pull request Sep 27, 2024

CCS Monero Signer Resurrection (milestone 4). UR offline signing data exchange. To merge, monero #9492 needs to be merged first. monero-project/monero-gui#4358

Open

selsta added pending review wallet labels Sep 28, 2024

sneurlax approved these changes Sep 30, 2024

View reviewed changes

SNeedlewoods reviewed Oct 9, 2024

View reviewed changes

SNeedlewoods reviewed Oct 14, 2024

View reviewed changes

DiosDelRayo mentioned this pull request Oct 23, 2024

Update PR: monero DiosDelRayo/DiosDelRayo.github.io#13

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications to enable Offline Signing in monero-gui and possible... #9492

Modifications to enable Offline Signing in monero-gui and possible... #9492

DiosDelRayo commented Sep 27, 2024

sneurlax left a comment

SNeedlewoods left a comment

SNeedlewoods Oct 9, 2024

SNeedlewoods Oct 9, 2024

SNeedlewoods Oct 9, 2024

SNeedlewoods Oct 9, 2024

SNeedlewoods Oct 9, 2024

SNeedlewoods left a comment

SNeedlewoods Oct 14, 2024

Modifications to enable Offline Signing in monero-gui and possible... #9492

Are you sure you want to change the base?

Modifications to enable Offline Signing in monero-gui and possible... #9492

Conversation

DiosDelRayo commented Sep 27, 2024

sneurlax left a comment

Choose a reason for hiding this comment

SNeedlewoods left a comment

Choose a reason for hiding this comment

SNeedlewoods Oct 9, 2024

Choose a reason for hiding this comment

SNeedlewoods Oct 9, 2024

Choose a reason for hiding this comment

SNeedlewoods Oct 9, 2024

Choose a reason for hiding this comment

SNeedlewoods Oct 9, 2024

Choose a reason for hiding this comment

SNeedlewoods Oct 9, 2024

Choose a reason for hiding this comment

SNeedlewoods left a comment

Choose a reason for hiding this comment

SNeedlewoods Oct 14, 2024

Choose a reason for hiding this comment